Support AGENT_CONFIG from volume mounts and remove secret logging #9243
Conversation
aayushchouhan09
requested review from
a team,
dannyzaken,
jackyalbo,
liranmauda and
nbecker-cibot
and removed request for
a team
WalkthroughAdds AGENT_CONFIG_PATH and AGENT_CONFIG to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Env as Environment
participant Init as noobaa_init.sh
participant Node as Node (noobaa-core/config.js)
participant FS as Filesystem
participant JQ as jq
participant OpenSSL as openssl
Init->>Env: read AGENT_CONFIG
alt AGENT_CONFIG unset
Init->>Node: node -e "require('.../config.js').AGENT_CONFIG"
Node-->>Init: AGENT_CONFIG
end
Init->>Init: ensure AGENT_CONFIG non-empty
alt agent_conf.json exists
Init->>FS: use existing agent_conf.json
else agent_conf.json missing
Init->>JQ: validate AGENT_CONFIG as JSON
alt valid JSON
Init->>FS: write AGENT_CONFIG -> agent_conf.json
else not JSON
Init->>OpenSSL: decode AGENT_CONFIG (base64)
OpenSSL-->>Init: decoded content
Init->>FS: write decoded -> agent_conf.json
end
end
note right of Init: logs "Got agent_conf" and "Written agent_conf.json"
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🪛 Shellcheck (0.11.0)src/deploy/NVA_build/noobaa_init.sh[warning] 128-128: Use 'cd ... || exit' or 'cd ... || return' in case cd fails. (SC2164) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config.js(1 hunks)src/deploy/NVA_build/noobaa_init.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
src/deploy/NVA_build/noobaa_init.sh
[warning] 128-128: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-package-lock-validation
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (1)
config.js (1)
245-250: LGTM! Clean implementation following existing patterns.The AGENT_CONFIG section follows the same pattern as other secret configurations (JWT_SECRET, SERVER_SECRET). Using
_get_data_from_fileis appropriate and handles missing files gracefully by returning undefined, which the shell script later validates.
| if [ -z ${AGENT_CONFIG} ]; then | ||
| cd /root/node_modules/noobaa-core/ | ||
| AGENT_CONFIG=$(node -e "const config = require('./config'); console.log(config.AGENT_CONFIG || '')") | ||
| fi |
There was a problem hiding this comment.
Quote variable and add error handling for cd command.
Two issues:
- Line 127:
${AGENT_CONFIG}should be quoted to handle empty strings correctly. - Line 128: Missing error handling if
cdfails.
Apply this diff:
- if [ -z ${AGENT_CONFIG} ]; then
+ if [ -z "${AGENT_CONFIG}" ]; then
- cd /root/node_modules/noobaa-core/
+ cd /root/node_modules/noobaa-core/ || exit 1
AGENT_CONFIG=$(node -e "const config = require('./config'); console.log(config.AGENT_CONFIG || '')")
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ -z ${AGENT_CONFIG} ]; then | |
| cd /root/node_modules/noobaa-core/ | |
| AGENT_CONFIG=$(node -e "const config = require('./config'); console.log(config.AGENT_CONFIG || '')") | |
| fi | |
| if [ -z "${AGENT_CONFIG}" ]; then | |
| cd /root/node_modules/noobaa-core/ || exit 1 | |
| AGENT_CONFIG=$(node -e "const config = require('./config'); console.log(config.AGENT_CONFIG || '')") | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 128-128: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 Prompt for AI Agents
In src/deploy/NVA_build/noobaa_init.sh around lines 127 to 130, the test and
directory change are unsafe: quote ${AGENT_CONFIG} in the if test (if [ -z
"${AGENT_CONFIG}" ]) to correctly handle empty/whitespace values, and add error
handling for the cd command (check its exit status and abort with an error
message and non-zero exit code if cd fails) before running the node command.
| echo "Got agent_conf" | ||
| if [ ! -f $AGENT_CONF_FILE ]; then | ||
| if echo "${AGENT_CONFIG}" | jq . >/dev/null 2>&1; then | ||
| echo "${AGENT_CONFIG}" >${AGENT_CONF_FILE} | ||
| else | ||
| openssl enc -base64 -d -A <<<${AGENT_CONFIG} >${AGENT_CONF_FILE} | ||
| fi | ||
| echo "Written agent_conf.json: $(cat ${AGENT_CONF_FILE})" | ||
| fi | ||
| echo "Written agent_conf.json" |
There was a problem hiding this comment.
Good security improvement, but quote variable in base64 decode.
Removing the direct echo of AGENT_CONFIG from logs is a good security practice. However, line 142 should quote ${AGENT_CONFIG} to handle values with spaces or special characters safely.
Apply this diff to line 142:
- openssl enc -base64 -d -A <<<${AGENT_CONFIG} >${AGENT_CONF_FILE}
+ openssl enc -base64 -d -A <<<"${AGENT_CONFIG}" >${AGENT_CONF_FILE}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "Got agent_conf" | |
| if [ ! -f $AGENT_CONF_FILE ]; then | |
| if echo "${AGENT_CONFIG}" | jq . >/dev/null 2>&1; then | |
| echo "${AGENT_CONFIG}" >${AGENT_CONF_FILE} | |
| else | |
| openssl enc -base64 -d -A <<<${AGENT_CONFIG} >${AGENT_CONF_FILE} | |
| fi | |
| echo "Written agent_conf.json: $(cat ${AGENT_CONF_FILE})" | |
| fi | |
| echo "Written agent_conf.json" | |
| echo "Got agent_conf" | |
| if [ ! -f $AGENT_CONF_FILE ]; then | |
| if echo "${AGENT_CONFIG}" | jq . >/dev/null 2>&1; then | |
| echo "${AGENT_CONFIG}" >${AGENT_CONF_FILE} | |
| else | |
| openssl enc -base64 -d -A <<<"${AGENT_CONFIG}" >${AGENT_CONF_FILE} | |
| fi | |
| fi | |
| echo "Written agent_conf.json" |
🤖 Prompt for AI Agents
In src/deploy/NVA_build/noobaa_init.sh around lines 137 to 145, the openssl
base64 decode uses an unquoted here-string which can break on spaces/special
characters; update the here-string to quote the variable (use
<<<"${AGENT_CONFIG}") so the entire AGENT_CONFIG value is passed safely to
openssl without word-splitting or globbing.
Signed-off-by: Aayush Chouhan <[email protected]>
Signed-off-by: Aayush Chouhan <[email protected]>
aayushchouhan09
force-pushed
the
agent-fix
branch
from
c5de7b9 to
17b14ea
Compare
3 participants
Describe the Problem
AGENT_CONFIGwas supported as environment variable, not as volume-mounted secrets. Also the agent logs exposed sensitive configuration data.Explain the Changes
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
Summary by CodeRabbit
New Features
Bug Fixes